Skip to content

[feature] Add support for user profiles API #724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[feature] Add support for user profiles API #724

wants to merge 3 commits into from

Conversation

mlnm-capco
Copy link

Adds support for the following User Profile CRUD operations (documented here)

GET /api/v2/users/{user_id}/profiles
GET /api/v2/user_profiles/{profile_id}
GET /api/v2/user_profiles?identifier={identifier}
PUT /api/v2/users/{user_id}/profiles?identifier={identifier}
PUT /api/v2/user_profiles?identifier={identifier}
PUT /api/v2/user_profiles/{profile_id}
DELETE /api/v2/user_profiles/{profile_id}

@mlnm-capco mlnm-capco mentioned this pull request Oct 30, 2024
Closed
@jamesforwardnwboxed
Copy link

This would be a great thing to have, @duemir @PierreBtz would you have an opportunity to get this reviewed soon? 🙌

PierreBtz
PierreBtz reviewed Dec 6, 2024
View reviewed changes
src/main/java/org/zendesk/client/v2/Zendesk.java
Comment on lines +1359 to +1361
public UserProfile getUserProfile(UserProfile userProfile) {
return getUserProfile(userProfile.getId());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the purpose of this method?

@PierreBtz
Copy link
Collaborator

PierreBtz commented Dec 6, 2024 •
edited
Loading

Thanks for the contribution and sorry for the delay in reviewing this. I'll take some time next week to work on this.
In the meantime, in the interest of speeding things up, I did a quick scan and didn't see any obvious blocker, I just have a question about one method.
I'll make a more thorough review later.

I'll also unblock the CI on this PR to have a run and make sure the test are passing. I can already tell you things will most likely fail because of the formatting, you should be able to format yourself with mvn spotless:apply. EDIT: I stand corrected, formatting is correct :)

@PierreBtz PierreBtz closed this Dec 6, 2024
@PierreBtz PierreBtz reopened this Dec 6, 2024
PierreBtz
PierreBtz requested changes Dec 12, 2024
View reviewed changes
Copy link
Collaborator

@PierreBtz PierreBtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!
I found minor quirks that I think we should address prior to merging, but I'm ok overall with the change.

src/main/java/org/zendesk/client/v2/model/UserProfile.java
private String source;
private String type;
private Date updatedAt;
private String userId;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks odd to use aString here where the API (public List<UserProfile> getUserProfilesForUser(long userId) {) works with a long.

src/main/java/org/zendesk/client/v2/Zendesk.java
}

public UserProfile createOrUpdateUserProfile(
UserProfile userProfile, String identifierType, String identifierValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about aligning with the UserProfile object and use an Identifier object here?

src/main/java/org/zendesk/client/v2/Zendesk.java
handle(UserProfile.class, "profile")));
}

public UserProfile getUserProfilebyIdentifier(String identifier) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about renaming the variable to queryIdentifier since this is not an identifier we are passing here, but an identifier query?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PierreBtz PierreBtz PierreBtz requested changes

@duemir duemir Awaiting requested review from duemir duemir is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mlnm-capco @jamesforwardnwboxed @PierreBtz